Skip to content

fix(engine): split contain_node proposed/eligible metric labels - #325

Merged
thejefflarson merged 1 commit into
mainfrom
thejefflarson/jef-766-contain_node-proposed-metric-double-counts-per-pass-edge
Aug 7, 2026
Merged

fix(engine): split contain_node proposed/eligible metric labels#325
thejefflarson merged 1 commit into
mainfrom
thejefflarson/jef-766-contain_node-proposed-metric-double-counts-per-pass-edge

Conversation

@thejefflarson

Copy link
Copy Markdown
Owner

Problem

record_contain_node("proposed", …) fired twice per pass for a newly-proposed, rails-clean ContainNode: once edge-triggered over ledger_delta.proposed (Engine::process's ledger-delta loop) and again level-triggered in the evaluate_proposal Proposed arm. Both call sites shared the same proposed label, so a fresh proposal was counted twice in that pass.

Approach chosen

Distinct labels (over dedup): the two events are genuinely different operator-facing signals —

  • proposed (unchanged): edge-triggered, fires once, the pass the ledger first surfaces the mitigation at all (fires even under mode: audit).
  • eligible (new): level-triggered, fires every pass the same mitigation stays armed at the node rung, in enforceScope, and every deterministic rail passes — the stronger claim "this is currently a rails-clean, actionable proposal". This term was already used in the surrounding doc comments ("ELIGIBLE to be surfaced as an actionable proposal"), so it lines up with existing vocabulary rather than inventing a new one.

Label vocabulary stays closed (proposed/eligible/reverted/rail_refused + fixed reasons) — no new per-node/high-cardinality dimension. No behavior change beyond the metric labels.

Testing

  • engine/src/engine/metrics.rs::tests::a_single_pass_never_double_counts_one_label — new focused unit test. The prior test for this counter was a no-op smoke test (constructs the no-op global meter, asserts no panic), so it couldn't have caught this. This test wires a local SdkMeterProvider + InMemoryMetricExporter (not the process-global meter, to avoid racing other tests in the same binary) via a new EngineMetrics::from_meter constructor, records one proposed + one eligible event (mirroring one pass over a single fresh proposal), and asserts both labels read back as exactly 1. Verified it fails (left: Some(2)) against the pre-fix code (both calls using "proposed") and passes after the fix.
  • Extended the existing smoke test to cover the eligible event.
  • cargo fmt --all -- --check, cargo clippy --all-targets --all-features -- -D warnings, cargo build --all-targets, cargo test --workspace all green (1213 passed, 0 failed, 2 ignored).

Closes JEF-766

The evaluate_proposal level-triggered arm (Engine::process) was reusing
the same "proposed" label as the ledger-delta edge-triggered surfacing,
so a single newly-proposed rails-clean ContainNode double-incremented
"proposed" in its first pass. Give the level-triggered signal its own
"eligible" label (already the term the surrounding doc comments used
for that state) instead of deduping — the two are genuinely distinct
operator-facing signals: "surfaced in the ledger" vs. "currently armed
+ in-scope + rails-clean this pass".

Adds a value-asserting unit test (via a local SdkMeterProvider +
in-memory exporter, not the process-global one) that fails on the old
double-count and passes with the label split.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@thejefflarson
thejefflarson enabled auto-merge (squash) August 7, 2026 01:05
@thejefflarson
thejefflarson merged commit cdfc8cc into main Aug 7, 2026
5 checks passed
@thejefflarson
thejefflarson deleted the thejefflarson/jef-766-contain_node-proposed-metric-double-counts-per-pass-edge branch August 7, 2026 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant